Fix spurious "Terminal needs approval" from Hermes pre_tool_call (#4985)#5010
Conversation
cmux's feed-event classifier maps Hermes `pre_tool_call` (a tool *starting*, no approval pending) to a blocking PermissionRequest whenever the tool is side-effecting (e.g. `terminal`). That fires a spurious "Terminal needs approval" notification while the Hermes TUI shows nothing to approve. Hermes reserves `pre_approval_request` for real approvals. This commit adds the regression test only (no fix) so CI proves the test catches the bug. classifyFeedEvent is made internal so it can be exercised directly. Refs #4985 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…_call Replace the raw event-name pattern matching in classifyFeedEvent with an explicit, typed registry keyed on (source, event) -> FeedEventSemantic. Notification eligibility and the blocking Feed wait are derived from the resolved semantic, never from string matching, so a tool-*starting* lifecycle event can no longer be mistaken for an approval request. The class of bug: the heuristic "pre-tool event + side-effecting tool => approval" is correct only for agents whose *only* signal is the pre-tool event (gemini, copilot, …). Agents with a dedicated approval event (Claude PermissionRequest, Codex PermissionRequest, Hermes pre_approval_request) must treat their pre-tool event as telemetry, or it double-counts and fires a spurious "needs approval" notification. Hermes `pre_tool_call` (tool starting, no approval) was being escalated to PermissionRequest for side-effecting tools like `terminal`, producing the false-positive banner in #4985. The registry encodes this distinction explicitly (toolStart vs toolStartMaybeApproval) and makes unknown/future event names safe by default: they resolve to non-actionable telemetry that never notifies. Fixes #4985 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughRefactors feed-event classification into: (1) resolve a typed FeedEventSemantic for (source,event) via per-source registries with generic fallback, then (2) map that semantic plus toolName to the wire hook_event_name and isActionable decision. Adds tests and Xcode project registration. ChangesFeed Event Classification Refactoring
Sequence DiagramsequenceDiagram
participant Caller as Feed hook (cmux hooks feed)
participant Classifier as FeedEventClassifier.classify
participant Lookup as feedEventSemanticRegistry
participant Mapper as wireMapping
participant Output as Notification system
Caller->>Classifier: (source, event, toolName)
Classifier->>Lookup: resolve FeedEventSemantic
Lookup-->>Classifier: FeedEventSemantic
Classifier->>Mapper: semantic, toolName
Mapper-->>Output: hook_event_name, isActionable
🎯 4 (Complex) | ⏱️ ~45 minutes
🚥 Pre-merge checks | ✅ 18✅ Passed checks (18 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@CLI/cmux.swift`:
- Around line 30239-30258: The switch branches for .approvalRequest and
.toolStartMaybeApproval duplicate the same special-case checks for
"ExitPlanMode" and "AskUserQuestion"; extract a small helper (e.g., a static
method like resolveSpecialToolName(toolName: String) -> (String, Bool) or a
private func specialToolResponse(toolName: String) -> (String, Bool)) and call
it from both cases, leaving the original branch logic to only handle the
default/path-specific behavior and the Self.sideEffectingTools check in
.toolStartMaybeApproval; update both case arms to delegate to this helper to
remove duplication.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: e7316230-3339-48fd-b6ca-df2457ee9b16
📒 Files selected for processing (3)
CLI/cmux.swiftcmux.xcodeproj/project.pbxprojcmuxTests/FeedEventClassificationTests.swift
Greptile SummaryExtracts
Confidence Score: 5/5Safe to merge. The registry-driven refactor is behavior-preserving for all existing agents, and the Hermes fix is surgically correct. All three registered agents (Claude, Codex, Hermes) have explicit, tested registry entries. The old switch ladders and the new registry produce identical results for every event combination that real agents emit. Unknown/future events now safely default to non-actionable telemetry rather than potentially mis-notifying. Test coverage is thorough and directly encodes the invariants that caused the original bug. No files require special attention. The one subtle difference — Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["FeedEventClassifier.classify(source, event, toolName)"] --> B["feedEventSemantic(source, event)"]
B --> C{source in registry?}
C -- yes --> D["agentTable[event] ?? .unknown"]
C -- no --> E["genericFeedEventSemantics[event] ?? .unknown"]
D --> F["FeedEventSemantic"]
E --> F
F --> G{semantic}
G -- ".approvalRequest" --> H["dedicatedApprovalEvent(toolName)\nor PermissionRequest, true"]
G -- ".toolStartMaybeApproval" --> I{sideEffecting tool?}
I -- yes --> J["PermissionRequest, true"]
I -- no --> K["PreToolUse, false"]
G -- ".toolStart" --> K
G -- ".toolEnd" --> L["PostToolUse, false"]
G -- ".statusNotification" --> M["Notification, false"]
G -- ".unknown" --> K
Reviews (4): Last reviewed commit: "Make Hermes feed-event regression test c..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
DRY the ExitPlanMode / AskUserQuestion tool-name routing shared by the .approvalRequest and .toolStartMaybeApproval wire mappings into a single dedicatedApprovalEvent(for:) helper, and scope FeedEventSemantic to private. Add coverage that the dedicated-approval tool names route correctly on the generic pre-tool path and that Codex's beforeShellExecution stays telemetry. Refs #4985 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…w feedback The FeedEventClassificationTests regression test never compiled on CI. `CMUXCLI.classifyFeedEvent` lives in the `cmux_cli` executable module, but the test (hosted by `cmux.app`) imported `cmux_DEV` and could neither see nor link the CLI tool's symbols. The `tests` job was failing at the cmuxTests *build* step (`cannot find 'CMUXCLI' in scope`) before any test ran — so the PR's two-commit red/green never actually validated the fix. Extract the pure classification logic into `CLI/FeedEventClassifier.swift` (`struct FeedEventClassifier`, single source of truth), compiled into both the `cmux-cli` and `cmuxTests` targets. The decision is now a genuine compiled unit test: 10 tests run and pass. `@testable import cmux_cli` is not an option — the app-hosted test bundle cannot link the CLI executable's symbols (the same reason the existing CLI test uses a subprocess). Behavior is unchanged for production payloads. Also addresses review feedback folded into the extracted file: - `FeedEventSemantic` is now `private` (Greptile P2). - `ExitPlanMode`/`AskUserQuestion` routing deduplicated via a shared `dedicatedApprovalEvent(for:)` helper (CodeRabbit). - Added coverage for Codex `beforeShellExecution` (telemetry) and generic-agent dedicated-approval-tool routing through the `.toolStartMaybeApproval` branch. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Fixed: the regression test never actually compiled on CIWhile iterating on this PR I found the Root cause: Fix (latest commit): extracted the pure classification logic into 🤖 Generated with Claude Code |
1 participant
Problem
With the Hermes hook integration installed (
cmux hooks hermes-agent install --yes), users occasionally see a "Terminal needs approval" desktop notification while the Hermes TUI shows no pending approval. Hermes' log showsshell hook timed out after 120s (event=pre_tool_call ...), confirmingpre_tool_callis the trigger.Root cause
CMUXCLI.classifyFeedEventmapped Hermespre_tool_call(a tool starting — no approval pending) to a blockingPermissionRequestwhenever the tool was side-effecting (e.g.terminal,Bash). That madeFeedCoordinatorpost the "needs approval" banner and block the hook for 120s. Hermes reserves a distinctpre_approval_requestevent for real approvals, sopre_tool_callshould never be treated as one.This is a class of bug, not a Hermes one-off. The heuristic "pre-tool event + side-effecting tool ⇒ approval" is correct only for agents whose only signal is the pre-tool event (gemini, copilot, factory, …). Agents that expose a dedicated approval event — Claude (
PermissionRequest), Codex (PermissionRequest), Hermes (pre_approval_request) — must treat their pre-tool event as telemetry, or it double-counts.Fix
Replaced the raw event-name
switchladders inclassifyFeedEventwith an explicit, typed registry keyed on(source, event) → FeedEventSemantic:.toolStart(dedicated-approval agents — always telemetry) vs.toolStartMaybeApproval(escalate side-effecting tools). Hermespre_tool_callis now.toolStart..unknown→ non-actionable telemetry that never notifies.pre_approval_requestkeeps firing the genuine approval notification through the dedicatednotificationhook subcommand path (unchanged); on the feed path it stays a non-blockingNotificationto avoid a duplicate banner. All other agents' mappings are behavior-preserving for real agent payloads.Tests
Two-commit red/green structure:
FeedEventClassificationTests(Swift Testing) added first — fails on current code.Coverage: Hermes
pre_tool_call/terminal/Bashare non-actionable;pre_approval_request/lifecycle events non-blocking; unknown future events safe; ClaudePreToolUsedoes not escalate whilePermissionRequest/ExitPlanMode/AskUserQuestiondo; generic agents still escalate side-effectingPreToolUse; CodexPreToolUseis telemetry.Fixes #4985
🤖 Generated with Claude Code
Need help on this PR? Tag
@codesmithwith what you need. Autofix is disabled.Note
Medium Risk
Changes when Feed blocks and notifies across all agents; behavior is intended to be preserving except the Hermes fix, but mis-registry entries could suppress real approvals or miss escalations for generic agents.
Overview
Fixes spurious "Terminal needs approval" notifications and 120s hook blocks when Hermes emits
pre_tool_call(tool start, not an approval) by replacing inlineclassifyFeedEventswitch logic in the CLI with sharedFeedEventClassifier.Classification now uses per-agent
(source, event) → FeedEventSemantictables, then maps semantics to wirehook_event_nameandisActionable. Agents with a dedicated approval event (Claude/CodexPermissionRequest, Hermespre_approval_request) treat pre-tool events as telemetry only (.toolStart); generic agents still escalate side-effecting pre-tool tools via.toolStartMaybeApproval. Unknown events default to non-actionable telemetry.Adds
FeedEventClassificationTestsand compiles the classifier into bothcmux-cliandcmuxTestsso behavior is regression-tested without subprocess CLI runs.Reviewed by Cursor Bugbot for commit 6d59f63. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by cubic
Stops false "Terminal needs approval" notifications from Hermes by treating
pre_tool_callas telemetry, not an approval. Prevents the 120s hook block and aligns event handling across agents. Fixes #4985.Bug Fixes
(source, event) → semanticregistry; notifications and blocking now derive from semantics.pre_tool_callto tool-start telemetry; keptpre_approval_requestas a non‑blocking feed notification (real approval still via thenotificationpath).beforeShellExecution(Codex) and other lifecycle hooks stay telemetry; tests cover Hermes, Claude, Codex, and generic agents.Refactors
FeedEventClassifierintoCLI/FeedEventClassifier.swiftand use it fromcmux.swift; compiled into bothcmux-cliand tests so unit tests run in CI.FeedEventSemantictoprivateand added a shareddedicatedApprovalEventhelper; expanded tests to cover dedicated-approval tool routing on generic pre‑tool paths.Written for commit 6d59f63. Summary will update on new commits.
Summary by CodeRabbit
Refactor
Tests